Add native ACP harness continuation#2046
Conversation
7bbb979 to
a20343f
Compare
xeophon
changed the title
19308e0 to
4d8f9a3
Compare
a20343f to
5d6c2be
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
2562cfb to
3a4ab43
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| if ! command -v curl >/dev/null 2>&1 || ! command -v npm >/dev/null 2>&1; then | ||
| { apt-get update -qq && apt-get install -y -qq curl ca-certificates nodejs npm >/dev/null; } \ | ||
| || apk add --no-cache curl ca-certificates nodejs npm >/dev/null | ||
| if [ -f /etc/alpine-release ]; then |
There was a problem hiding this comment.
🟡 Medium pi/harness.py:45
On Alpine, the install script uses the distribution's unpinned nodejs package and then aborts if it is older than 22.19, instead of downloading the pinned VF_PI_NODE_VERSION like the non-Alpine branch does. On Alpine images whose repository ships an older Node, setup fails with "Pi requires Node.js 22.19 or newer" rather than installing the required version. Consider applying the same pinned-Node download logic to the Alpine branch instead of relying on the system package.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @verifiers/v1/harnesses/pi/harness.py around line 45:
On Alpine, the install script uses the distribution's unpinned `nodejs` package and then aborts if it is older than 22.19, instead of downloading the pinned `VF_PI_NODE_VERSION` like the non-Alpine branch does. On Alpine images whose repository ships an older Node, `setup` fails with "Pi requires Node.js 22.19 or newer" rather than installing the required version. Consider applying the same pinned-Node download logic to the Alpine branch instead of relying on the system package.
ApprovabilityVerdict: Needs human review Unable to check for correctness in 2280387. This PR introduces a new ACP protocol feature affecting multiple harnesses with significant new logic. Multiple unresolved review comments identify potential bugs including a P1 issue with message ID handling that could break ACP harness segments. You can customize Macroscope's approvability policy. Learn more. |
3c8abb2 to
388137c
Compare
3a4ab43 to
ea18c95
Compare
xeophon
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea18c958c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mcp_args: list[str] = [] | ||
| restore_home = f'export HOME="${HOME_VAR}"; unset {HOME_VAR}; ' |
There was a problem hiding this comment.
When a Pi rollout has no MCP servers, this wrapper prologue is emitted before exec pi ..., but it sets HOME to the literal string VF_PI_ORIGINAL_HOME rather than the value saved by ACP_COMMAND before it rewrites HOME to the agent dir. That makes every non-MCP Pi segment run with a bogus relative home, so Pi and model-executed commands that rely on home-scoped config/cache/state no longer see the runtime's real home; use $VF_PI_ORIGINAL_HOME here instead.
Useful? React with 👍 / 👎.
| name: {"disabled": True} for name in self.config.disabled_tools or [] | ||
| }, | ||
| } | ||
| await runtime.write(SETTINGS_PATH, json.dumps(settings).encode()) |
There was a problem hiding this comment.
Pool decline exit unhandled
Medium Severity
The Pool harness no longer maps exit code 4 to success after switching to pool acp via POOL_ACP.run. When the agent declines a task with that code, Harness.run now treats a non-zero exit as failure and raises HarnessError instead of returning a successful ProgramResult.
Reviewed by Cursor Bugbot for commit 9d03dbbce1cc468f7cd7eb38b6223cad483b1c01. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d03dbbce1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| update.content, TextContentBlock | ||
| ): | ||
| return | ||
| if update.message_id is not None and update.message_id != self.message_id: |
There was a problem hiding this comment.
ACP v1 text chunks from the pinned agent-client-protocol==0.11.0 schema do not expose message_id, so the first normal AgentMessageChunk delivered by Pool/Kimi/Pi causes this callback to raise AttributeError before visible_reply is accumulated. In that case connection.prompt() fails and every native ACP harness segment errors out as soon as the agent streams text; use getattr(update, "message_id", None) or otherwise treat a missing ID as a single message.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9f1192f. Configure here.
9f1192f to
2280387
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2280387cdb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| prompt_path = f"{agent_dir}/prompt.txt" | ||
| await runtime.write(prompt_path, prompt.encode()) | ||
| await runtime.write(f"{agent_dir}/models.json", json.dumps(models).encode()) |
There was a problem hiding this comment.
When starting a new Pi ACP session, pi-acp emits its default startup-info block as an agent_message_chunk immediately after session/new; because this harness never writes Pi settings to set quietStartup, that out-of-band chunk can be captured by the generic ACP runner as the segment's visible_reply. In Agent.chat() this makes the first Pi reply include the startup banner (or even satisfy the “produced a reply” check) instead of just the model's answer, so Pi-driven multi-turn/user-sim flows receive polluted assistant text.
Useful? React with 👍 / 👎.
2 participants
Overview
Adds user-simulation continuation across v1 harnesses on top of the unified agent chat exchange in #2049, using native ACP only where the underlying agent supports it.
Details
vf.ACPas the public native ACP transport and surfaces the ACP-visible agent response throughChatSession.pi-acpadapter for persistent Pi sessions, with full-transcript recovery when the session marker is missing.Note
Add native ACP harness continuation for Kimi, Pi, Pool, and RLM agents
ACPclass and runner script that executes ACP agents, captures streaming assistant text asvisible_reply, handles session resume, and auto-approves permissions.--printmode, enabling session handling and structured reply capture.visible_reply: str | NonetoProgramResultso ACP-based harnesses can surface assistant output to callers.Harness.runnow returnsProgramResultand only raisesHarnessErroron non-zero exit when no stop condition is set;RolloutRun.stepnow returnsProgramResult | Noneinstead ofbool.launchhandlers now acceptdata: TaskDataand resolve prompts viaself.resolve_prompt(data)rather than reading directly fromtrace.task.data.step()returningboolwill break.Macroscope summarized 2280387.
Note
Medium Risk
Switches several agent harnesses from print-mode CLIs to ACP/session semantics and changes
step()'s return type—multi-turn rollouts and callers ofstep()may behave differently; install paths now pull npm/Node and external agent binaries.Overview
Introduces
vf.ACPas the shared Agent Client Protocol runner (uv script + session resume, MCP wiring, streamed assistant text asvisible_replyonProgramResult). Kimi Code, Pi, and Pool no longer use one-shot print/exec modes—they launch nativeacpsubprocesses with per-rollout session files soAgent.chat()can continue exchanges via the defaultHarness.resumerelaunch (Pi viapi-acp, npm/Node 22.19+ install).RLM opts into
SUPPORTS_MESSAGE_PROMPTand JSON-serializesMessagesprompts for stateless transcript replay; compact/claude launch paths takeTaskDataandresolve_prompt(data).Harness.runreturnsProgramResult;RolloutRun.stepreturnsProgramResult | None(replacingbool), andChatSession.turnprefersvisible_replyover trace text when present.Reviewed by Cursor Bugbot for commit 2280387. Bugbot is set up for automated code reviews on this repo. Configure here.